-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove .mbdf and .mbd #1762
Remove .mbdf and .mbd #1762
Conversation
As this is a breaking change, we won't be able to do any v3.* releases (unless as hot patches to previous release) after merging this PR, right? |
@damithc Actually I'm not too sure about this maybe @ang-zeyu can respond? I think its good SWE practice to release this as a new version though? Since this will indeed remove all support for existing .mbdf and .mbd files. Perhaps I can include a new markbind command to fix all the old .mbdf and .mbd files? Thoughts on this? |
…move-mbd/mbdf
@ong6 at the same time, can you insert a tip somewhere in the user docs to guide authors on how to exclude fragment files from rendering? |
Yup. Will need to create a separate branch for hot patches releases (if any) after this is merged. |
Co-authored-by: Liu YongLiang <[email protected]>
Co-authored-by: Liu YongLiang <[email protected]>
@tlylt @jovyntls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @ong6!
Thanks @tlylt and @jovyntls for the reviews 👍
Since this PR is a breaking change, perhaps we can have a more detailed proposed commit message to explain a little bit more about the context?
I've made the changes, perhaps instead of a more detailed explanation in the commit message, I will update the highlight/discussion part instead?
I think it's better to put it in the commit message as it will be easier to track from the commit history.
packages/cli/test/functional/test_site/sub_site/nested_sub_site/site.json
Outdated
Show resolved
Hide resolved
...i/test/functional/test_site/expected/sub_site/nested_sub_site/testNunjucksPathResolving.html
Outdated
Show resolved
Hide resolved
Thanks for the review @jonahtanjz I've made all the suggested changes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes @ong6
Just 2 more comments :)
...s/cli/test/functional/test_site/sub_site/nested_sub_site/testNunjucksPathResolvingInclude.md
Outdated
Show resolved
Hide resolved
@@ -2,7 +2,7 @@ const path = require('path'); | |||
const fs = require('fs-extra'); | |||
const ensurePosix = require('ensure-posix-path'); | |||
|
|||
const markdownFileExts = ['.md', '.mbd', '.mbdf']; | |||
const markdownFileExts = '.md'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have to update isMarkdownFileExt method as well since it is no longer an array. Can use ===
instead of includes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this will give false positives? Although unlikely, but files with extension .m
or having no extension will be returned as true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh ok I see, will implement the change then!
…e/testNunjucksPathResolvingInclude.md Co-authored-by: Jonah Tan <[email protected]>
…move-mbd/mbdf
…to remove-mbd/mbdf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ong6 :) You may want to update the commit message as well following this template.
Other than that the rest looks good 👍
Co-authored-by: Jonah Tan <[email protected]>
Oh nice spot, didn't realize that issue also! Thanks for checking my PR once again. I have updated the commit message as well! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ong6 :)
LGTM 👍
What is the purpose of this pull request?
Fixes #1672
Overview of changes:
Removes allowing of
.mbdf
and.mbd
as file extension types.reusing contents
calledCreating Custom Fragments
, view here.mbdf
and.mbd
.mbdf
and.mbd
..mbdf
and.mbd
custom-fragment.md
Anything you'd like to highlight / discuss:
Some context for this PR:
.mbd
was created with the intent of making our own extension called .markbind.mbdf
was created to easily exclude fragments from page generationpagesExclude
with PR Add site.json properties for file exclusions #1328, the definition of fragments can be described by the users hence rendering the first 2 points obsolete.Testing instructions:
npm run test
inpackages/cli
Proposed commit message: (wrap lines at 72 characters)
Remove
.mbdf
and.mbd
as markbind supported file extensions.Syntax parsers do not work with
.mbdf
and.mbd
andthis functionality could be implemented using other means (e.g. pagesExclude)
Refer to this for the full disscussion #1672.
Checklist: ☑️